-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TieredStorage] Boundary check for get_account_offset() #34531
[TieredStorage] Boundary check for get_account_offset() #34531
Conversation
7b8cc2e
to
ed7368a
Compare
ed7368a
to
3aca41a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34531 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 822 822
Lines 221540 221607 +67
=========================================
- Hits 181403 181395 -8
- Misses 40137 40212 +75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Feel free to either fix the nit here, or merge as-is and fix all the nits together in a subsequent PR (so you don't have to wait through CI again).
@@ -83,13 +83,13 @@ impl IndexBlockFormat { | |||
footer: &TieredStorageFooter, | |||
index_offset: IndexOffset, | |||
) -> TieredStorageResult<&'a Pubkey> { | |||
let account_offset = match self { | |||
let offset = match self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR probably shouldn't modify anything in get_account_address()
, right? That's being handled here: #34529. Once that PR is merged, I'm not sure if there will be file conflicts here or not. I guess we'll see!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think this PR will have conflicts anyway as their tests are added in the same line. I am just trying to make the PR self-contained. Will do a rebase as #34529 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
debug_assert!( | ||
offset.saturating_add(std::mem::size_of::<Offset>()) | ||
<= footer.owners_block_offset as usize, | ||
"reading IndexOffset ({}) would exceeds index block boundary ({}).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same nit here:
"reading IndexOffset ({}) would exceeds index block boundary ({}).", | |
"reading IndexOffset ({}) would exceed index block boundary ({}).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix this one in the same PR as this PR needs a rebase anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
c4e59ba
to
da602bd
Compare
.open(&path) | ||
.unwrap(); | ||
let mmap = unsafe { MmapOptions::new().map(&file).unwrap() }; | ||
// IndexOffset does not exceeds the account_entry_count but exceeds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #34546
}; | ||
|
||
{ | ||
// we only writes a footer here as the test should hit an assert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #34546.
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
…34531) #### Problem TieredStorage doesn't perform boundary check in get_account_offset when the input IndexOffset isn't valid. #### Summary of Changes This PR adds two checks. First, it checks whether the IndexOffset exceeds the boundary of the index block. Second, when an index format that has the same index entries as account entries is used, it also checks whether IndexOffset is smaller than account_entry_count. #### Test Plan Two new tests are added to this PR.
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065)
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143)
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344)
…labs#33964) [TieredStorage] Improve param naming of IndexBlockFormat (solana-labs#34033) [TieredStorage] HotStorageReader::get_account_offset (solana-labs#34031) [TieredStorage] Rename owners_offset to owners_block_offset (solana-labs#34047) [TieredStorage] HotStorageReader::get_account_address (solana-labs#34032) [TieredStorage] OwnersBlock (solana-labs#34052) [TieredStorage] HotStorageReader::get_owner_address (solana-labs#34053) [TieredStorage] Define OwnerOffset as u32 (solana-labs#34105) [TieredStorage] Use OwnerOffset type in TieredAccountMeta (solana-labs#34106) Refactors TieredStorageFile read/write methods (solana-labs#34147) [TieredStorage] Make IndexBlock persist u32 offsets (solana-labs#34133) [TieredStorage] Make IndexOffset use u32 (solana-labs#34152) Move MatchAccountOwnerError from append_vec to accounts_file (solana-labs#34187) [TieredStorage] Make AccountOffset use u32 (solana-labs#34151) [TieredStorage] Allow HotStorage to handle more account data (solana-labs#34155) [TieredStorage] Make AccountOffset a trait, introduce HotAccountOffset (solana-labs#34335) [TieredStorage] Improve comments for HOT_ACCOUNT_ALIGNMENT (solana-labs#34404) [TieredStorage] Unit-tests for checking invalid HotAccountOffset (solana-labs#34376) [TieredStorage] Boundary check for accessing hot account meta (solana-labs#34349) [TieredStorage] boundary check for get_account_address() (solana-labs#34529) Sanitizes tiered storage footer after reading from disk (solana-labs#34200) Adds read/write/get_pod() fns to tiered storage (solana-labs#34415) Uses consistent error types in tiered storage (solana-labs#34110) [TieredStorage] Boundary check for get_account_offset() (solana-labs#34531) [TieredStorage] HotStorageReader::account_matches_owners (solana-labs#34350) [TieredStorage] Fix typos in index.rs (solana-labs#34546) [TieredStorage] HotAccountsReader::get_account (solana-labs#34499) [TieredStorage] Rename AddressAndBlockOffsetOnly to AddressesThenOffsets (solana-labs#34658) [TieredStorage] HotStorageWriter::new() (solana-labs#34659) [TieredStorage] Include executable field into AccountMetaFlags (solana-labs#34724) [TieredStorage] Code refactoring for OwnersBlock (solana-labs#34854) [TieredStorage] In-memory struct for writing OwnersBlock (solana-labs#34853) [TieredStorage] writing hot account blocks and index blocks (solana-labs#34828) [TieredStorage] Use RENT_EXEMPT_RENT_EPOCH in HotStorageWriter (solana-labs#34950) [TieredStorage] Write owners block for HotAccountStorage (solana-labs#34927) [TieredStorage] Avoid AccountHash copy in AccountMetaOptionalFields (solana-labs#34969) [TieredStorage] Correct the HotStorage API for account_matches_owners (solana-labs#34967) [TS] Add get_account() and account_matches_owner() to TieredStorageReader (solana-labs#34968) [TieredStorage] Have HotStorageWriter::write_account() return Vec<StoredAccountInfo> (solana-labs#34929) [TieredStorage] Use IndexOffset in TieredStorageMeta and get_account() (solana-labs#35046) [TieredStorage] TieredStorageReader:: and HotStorageReader:: accounts() (solana-labs#35031) [TieredStorage] Enable hot-storage in TieredStorage::write_accounts() (solana-labs#35049) [TieredStorage] Put commonly used test functions into test_utils.rs (solana-labs#35065) [TieredStorage] Make TieredStorage::write_accounts() thread-safe (solana-labs#35143) [TieredStorage] rent_epoch() returns 0 for zero-lamport accounts (solana-labs#35344) [TieredStorage] Deprecate the use of account-hash in HotStorage (solana-labs#93)
Problem
TieredStorage doesn't perform boundary check in get_account_offset
when the input IndexOffset isn't valid.
Summary of Changes
This PR adds two checks. First, it checks whether the IndexOffset exceeds
the boundary of the index block. Second, when an index format that has the
same index entries as account entries is used, it also checks whether IndexOffset
is smaller than account_entry_count.
Test Plan
Two new tests are added in this PR.